Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Support for legacy source prop when value is TemplateLiteral #245

Merged
merged 3 commits into from
Dec 18, 2024

Conversation

xeho91
Copy link
Collaborator

@xeho91 xeho91 commented Dec 14, 2024

Credits for debugging/finding goes to @dominikg via Storybook Discord

There's only one question: Should we call .trim() on the extracted template literal value?
EDIT. Based on this Storybook test preview (bottom-right corner - Show code) it seems whitespaces are already handled internally in the core. I'm guessing. So, the answer might be no.

@xeho91 xeho91 added bug Something isn't working patch Increment the patch version when merged next Related to the next version labels Dec 14, 2024
@xeho91 xeho91 requested a review from JReinhold December 14, 2024 09:44
@xeho91 xeho91 changed the base branch from main to next December 14, 2024 09:45
@JReinhold
Copy link
Collaborator

Why are we turning the original source prop into a string? In reality, aren't we just "moving" the prop to somewhere else? And if so, can't we just move the whole expression, not caring about its type?

What is stopping this from working:

from v4 syntax

<Story source={generatedSource()} />

to v4 syntax

<Story parameters={{ docs: { source: { code: generatedSource() } } }} />

I'm sorry for my ignorance.

@xeho91
Copy link
Collaborator Author

xeho91 commented Dec 15, 2024

Do you mean moving the entire AST node object for whatever value is set inside source?

Well, that's easy. Let's break down the type of what AST.Attritbue.value can be:

  1. true - easy to move, no problem, valid JavaScript node
  2. AST.ExpressionTag - easy to move, no problem, valid JavaScript node
  3. (AST.Text | AST.ExpressionTag)[] - nope

The 3rd one, I really don't like it. Is one of the valid Svelte syntax, and I wouldn't be surprised if maintainers ever regret inventing this one too now.

This this "feature":

<Component prop="Some text with dynamic {value}, and can have more {values}" />

From user perspective, meh. From AST perspective, I don't love it 😅

@xeho91
Copy link
Collaborator Author

xeho91 commented Dec 15, 2024

On the second thought... it becomes array (AST.Text | AST.ExpressionTag)[] only when the aforementioned pattern for value is used. 🤔

Meaning if there's no 'interpolation' via ExpressionTag inside the string, it becomes just a single ExpressionTag with a literal of the string value.

<!-- (AST.Text | AST.ExpressionTag)[]   👇 because of          and 👇       -->                                 
<Component prop="Some text with dynamic {value}, and can have more {values}" />
<!--             👇 this is just an AST.ExpressionTag -->                           
<Component prop="Some text with literal value" />

So, what we could do is omit/throw error when value is an array and just forward the AST node for attribute value into object expression (parameters.docs.source.code), and let the Storybook core handle invalid schema. Is that what you expect?

@JReinhold
Copy link
Collaborator

JReinhold commented Dec 18, 2024

To me it looks like that (AST.Text | AST.ExpressionTag)[] is just a fancy template literal, and in fact that is exactly what Svelte turns it into, it doesn't even look up the variables, see line 10 the JS output here:

https://svelte.dev/playground/hello-world?version=5.14.3#H4sIAAAAAAAAE22NQQrCMBQFrxLeuijdhlrwDu6si1S-JpAmIXm1Stu7S0V3bocZZkYwg0DjJEPyhqIKswt35YIyZHb9SEGFm_NSoM8z-EqbvwFUv_qY0q48xH_c3hT5x68xUAILNBpbKzp6OXSY4qTmEgeh3cbLoiKt5LVD-y2ava1bVKA8Cc08ynpZ39uGYQC6AAAA

from

"wow {something || other}"

to

`wow ${(something || other) ?? ""}`

Could we do the same? Maybe there's some code we can lift directly from the Svelte repo.

Regardless, I'm not sure how complicated that would be, and this fix here is better than nothing, I'm just exploring if we can make it even more dynamic and try to be less smart, but that shouldn't block this PR.

@JReinhold JReinhold merged commit 6954b85 into next Dec 18, 2024
7 checks passed
@xeho91
Copy link
Collaborator Author

xeho91 commented Dec 18, 2024

Let me see if I can find in their internals a function that we can copy. Thanks for the idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working next Related to the next version patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants